fix(transaction): fix correctness bugs in OverwriteAction - #81

Summary

Our OverwriteAction (crates/iceberg/src/transaction/overwrite.rs) was originally built on top of upstream's in-progress CoW work, apache/iceberg-rust#2185. That PR picked up a detailed review (review) flagging several correctness issues. I checked each one against our own implementation — three applied here too and are fixed in this PR; one was already handled correctly on our side; one (summary counts) is fixed one level up in shared code.

Fixes

1. Resurrection bug (the serious one). rewrite_manifest's fallback branch called add_existing_entry for every entry not newly deleted this round — including entries already Deleted by a prior overwrite. add_existing_entry unconditionally resets status to Existing, so a later overwrite that touches the same manifest for an unrelated reason silently resurrected previously-deleted files as live data. Fixed by routing already-non-alive entries through add_deleted_entry instead, preserving their original snapshot_id as a tombstone. Added test_second_overwrite_does_not_resurrect_deleted_file (fails without the fix).

2. Wrong schema-id on rewritten manifests. Schema was taken from table.metadata().current_schema() — the table's current schema — rather than the manifest being rewritten's own schema. After any schema evolution between the original write and a later overwrite, this stamped the wrong schema-id on old entries, which schema-id-aware readers (Java, PyIceberg) would misinterpret. Fixed by deriving both schema and partition spec directly from the manifest's own ManifestMetadata (already-resolved objects, not just IDs) — simpler than the partition_spec_by_id table lookup it replaces, and doesn't depend on the table still listing that partition spec.

3. Manifest naming/location bypassed convention. Rewritten manifests were written to a hardcoded {location}/metadata/ path with a fresh random UUID per manifest, instead of metadata_location() (which respects a configured write.metadata.path table property, already used correctly elsewhere in this file for the manifest list) and the commit's own UUID (shared across every manifest touched by one commit, matching SnapshotProducer::new_manifest_writer's convention). Added a commit_uuid() getter on SnapshotProducer and an index parameter to rewrite_manifest to keep names unique when a commit rewrites more than one manifest.

4. Partial-overwrite summary counts (shared code). SnapshotProducer::summary() unconditionally applied "truncate full table" semantics (replace computed added/removed counts with the previous snapshot's totals) for any Operation::Overwrite — wrong for OverwriteAction's explicit-file-list partial overwrites, which already know exactly what was added/removed. Added a truncate_full_table() method to SnapshotProduceOperation (default false, only meaningful for an operation that genuinely replaces the whole table); OverwriteOperation opts out explicitly. test_delete_only_overwrite_summary, which had documented the old (wrong) truncated counts as expected, now asserts the correct ones.

Not applicable: the review's manifest-filtering concern (dropping delete-only manifests) — confirmed both FastAppendAction and OverwriteAction::existing_manifest() already keep manifests with has_deleted_files().

Test plan

• cargo test -p iceberg --lib — 1625 passed, 0 failed
• cargo test -p iceberg --lib transaction:: — 89 passed, including the new regression test
• cargo clippy -p iceberg --all-targets -- -D warnings — clean
• cargo fmt -p iceberg -- --check — clean
• cargo check --workspace --all-features --all-targets (excl. python bindings) — clean

🤖 Generated with Claude Code